[MNG-8768] Add executable() function for conditional profile activation based on PATH#12332
[MNG-8768] Add executable() function for conditional profile activation based on PATH#12332Hiteshsai007 wants to merge 3 commits into
Conversation
…on based on PATH Implements MNG-8768 (GitHub apache#10115): adds an executable(name_or_path) function to Maven's condition-based profile activation DSL. The function evaluates to true if the given executable name can be found in the system PATH, or if an absolute/relative path points directly to an executable file. Use-case (from the issue): auto-detect the presence of x86_64-linux-musl-gcc when building GraalVM native images with the native-maven-plugin, and conditionally add --static --libc=musl to the build options only when the compiler wrapper is available on the current machine. Example pom.xml usage: <activation> <condition>executable('x86_64-linux-musl-gcc')</condition> </activation> Implementation details: - ConditionFunctions.executable() delegates to new ExecutableFinder helper - ExecutableFinder splits the env.PATH system property and probes each directory; on Windows it also tries .exe / .cmd / .bat / .com extensions - Absolute/relative paths (containing a path separator) are checked directly without PATH traversal - PATH is sourced from ProfileActivationContext.getSystemProperty('env.PATH') (Maven's normalised env var key), with a System.getenv('PATH') fallback Tests: - 5 new end-to-end tests in ConditionProfileActivatorTest (MNG-8768) - 12 new unit tests in ExecutableFinderTest covering PATH search, Windows extension probing, absolute paths, non-executable files, and empty PATH
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Fully automatic review from Claude Code
Thanks for the contribution — the use case (auto-detecting x86_64-linux-musl-gcc for GraalVM native images) is real and well-motivated. However, I have significant concerns about the current approach, primarily around build reproducibility.
Core concern: environment-dependent profile activation in published POMs
The executable() function makes profile activation depend on the runtime PATH, which is inherently non-reproducible. When a POM containing <condition>executable('x86_64-linux-musl-gcc')</condition> is published to a repository and consumed by other projects, the effective model silently changes depending on what's installed on each consumer's machine.
Today, Maven does not strip <condition> expressions from consumer POMs — they survive as-is into published artifacts (verified by reading DefaultConsumerPomBuilder). This means:
- Two developers building the same project get different dependency trees depending on what's on their
PATH - CI and local builds silently diverge
- There's no audit trail — no logging tells you "profile X activated because executable Y was found at Z"
While existing activation mechanisms (<os>, <jdk>, <property>) are also environment-dependent, they operate on a small set of well-defined, relatively stable values that flow through ProfileActivationContext. executable() is qualitatively different — PATH is per-session, per-user, per-shell, and varies wildly across machines.
Additional technical issues
1. System.getenv("PATH") fallback bypasses the activation context
In ExecutableFinder.getPathValue() (line 130), there's a System.getenv("PATH") fallback when env.PATH isn't in the context. This is the only place in the profile activation code that reaches directly into the OS environment, breaking the abstraction that all other activators respect. Every other activator uses context.getSystemProperty() exclusively.
2. System.getProperty("os.name") bypasses the context for OS detection
ExecutableFinder.isWindows() (line 161) calls System.getProperty("os.name") directly, while the existing OperatingSystemProfileActivator reads os.name through context.getSystemProperty(). This means in test or remote-build scenarios where the context simulates a different OS, the executable finder would still use the real OS.
3. java.io.File usage instead of NIO2
The code uses File.separator, File.pathSeparator, and Paths.get() — the codebase has been migrating to NIO2 Path APIs (see PR #11364). Should use Path.of() and java.nio.file equivalents.
4. Dead code
candidateNames() (lines 134–145) is package-private, exposed for tests, but never used by isExecutableInPath(). The candidate logic is duplicated inline in the main method.
5. Test issues
returnsFalseWhenFileIsNotExecutable()conditionally skips on Windows viaSystem.getProperty("os.name")instead of@DisabledOnOs(WINDOWS)getPathValueFallsBackToSystemEnv()testsSystem.getenv("PATH") == System.getenv("PATH")— always passes, doesn't prove correctnesstestExecutableWithAbsolutePath()does.replace('\\', '/')which changes path semantics on Windows
Possible ways forward
-
Restrict to build-time only: ensure
executable()conditions are stripped from consumer POMs duringDefaultConsumerPomBuilderprocessing, similar to how<build>is stripped from profiles. This would make the function usable for local build decisions without polluting published POMs. -
Require explicit opt-in: only allow
executable()in profiles that are marked as non-publishable or build-local, so users must consciously decide whether the condition should survive into the consumer POM. -
Add a model validator warning: emit a warning during
mvn install/mvn deploywhen a consumer POM containsexecutable()conditions, alerting users that the published POM depends on the consumer's environment. -
Document as build-local only: if the function is accepted, document clearly that it should only be used in local profiles and not in POMs intended for publication. Though documentation alone doesn't prevent misuse.
Any of options 1–3 (or a combination) would address the reproducibility concern while preserving the useful functionality.
- Add a warning in DefaultModelValidator if executable() condition is used, noting reproducibility issues
- Add a Javadoc warning to ConditionFunctions.executable()
- Remove System.getenv('PATH') fallback in ExecutableFinder
- Fix OS checking to use ProfileActivationContext instead of System.getProperty
- Convert Paths.get to Path.of
- Remove dead code (candidateNames)
- Update tests: remove fallback test, use @DisabledOnOs(OS.WINDOWS) for OS-specific test
|
Thanks for the detailed review! I completely agree with the reproducibility concerns and the technical points. I've pushed an update that addresses all of them. Reproducibility / Model ValidationTo address the core concern about environment-dependent profile activation in published POMs, I went with Option 3 + Option 4:
Technical Fixes
Everything is pushed to the branch! Let me know if there's anything else needed. |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Fully automatic review from Claude Code
Thanks for the quick turnaround. Most of the technical issues from the first review are properly addressed:
- ✅ Removed
System.getenv("PATH")fallback - ✅ OS detection now uses
ProfileActivationContext - ✅
Paths.get()→Path.of() - ✅ Dead code (
candidateNames()) removed - ✅
@DisabledOnOs(OS.WINDOWS)used correctly - ✅ Removed trivial fallback test
- ✅ Model validator warning added
- ✅ Javadoc documentation added
A few items remain.
Accidental file: your-database.db
There's a zero-byte your-database.db file committed at the repo root. This is clearly not intended — please remove it from the branch.
.replace('\\', '/') still present in integration test
In ConditionProfileActivatorTest.testExecutableWithAbsolutePath(), the .replace('\\', '/') that was flagged in the first review is still there. It was correctly removed from ExecutableFinderTest.findsExecutableByAbsolutePath(), but the integration test still has it.
Windows code paths have zero test coverage
The contextWithPath() test stub never sets os.name, so isWindows() always returns false in tests. This means the Windows extension probing logic (.exe, .cmd, .bat, .com) is completely untested. Consider adding at least one test that simulates a Windows environment by including os.name set to Windows 10 in the context properties and verifying that e.g. my-tool.exe is found when searching for my-tool.
isExecutableFile() comment is misleading for direct paths
The comment on isExecutableFile() says "we are already filtering by extension in the caller" — but this is only true for the PATH-search branch. When the user provides a direct path (containing a separator), there is no extension filtering. This means executable('C:/path/to/readme.txt') would return true on Windows for any regular file. The comment should be corrected, and you may want to consider whether direct paths on Windows should also require an executable extension.
Regarding reproducibility (Options 3+4)
The warning in DefaultModelValidator and the Javadoc are reasonable first steps. One gap worth noting: the warning fires at build time of the POM that contains the executable() condition, but when a consumer pulls that POM from a repository and builds with it, the condition silently activates (or not) based on the consumer's PATH — with no warning on the consumer side. Stripping executable() conditions from consumer POMs (Option 1 from the first review) would close this gap, but that's a larger change that could come as a follow-up.
| Files.createFile(fakeExec); | ||
| fakeExec.toFile().setExecutable(true); | ||
|
|
||
| String absPath = fakeExec.toAbsolutePath().toString().replace('\\', '/'); |
There was a problem hiding this comment.
The .replace('\\', '/') is still here — it was removed from ExecutableFinderTest but not from this integration test. This was flagged in the first review.
| // because we are already filtering by extension in the caller. On Unix we rely on the | ||
| // execute bit. | ||
| return isWindows || Files.isExecutable(path); | ||
| } |
There was a problem hiding this comment.
The comment above (lines 136-138) says "we are already filtering by extension in the caller" — but that's only true for the PATH-search branch (lines 88-95). For direct paths (line 69), the caller doesn't filter by extension, so on Windows isExecutableFile returns true for any regular file regardless of extension.
Also, for direct paths on Windows, the extension probing (name + ".exe", etc.) is not applied. Consider whether executable('/path/to/tool') on Windows should also try /path/to/tool.exe.
…fix direct-path comment
- Remove accidental your-database.db from repo root
- Remove .replace('\\', '/') from ConditionProfileActivatorTest.testExecutableWithAbsolutePath()
- Fix isExecutableFile() comment: no longer claims caller always filters by extension
- Direct paths on Windows now also probe .exe/.cmd/.bat/.com extensions
- Add contextWithPathAndOs() test helper that sets os.name in context
- Add 3 new Windows-simulated tests:
- findsExecutableWithWindowsExtensionInPath: PATH search finds my-tool.exe for 'my-tool'
- findsExecutableWithWindowsExtensionByDirectPath: direct path probes .exe
- doesNotAppendExtensionOnNonWindows: confirms no extension probing on Linux
|
Thanks for the thorough second review! All items addressed:
Regarding consumer POM stripping (Option 1) — agreed that's a good follow-up. Happy to tackle it in a separate PR once this one lands. |
2 participants
Implements MNG-8768 (GitHub #10115): adds an executable(name_or_path) function to Maven's condition-based profile activation DSL.
The function evaluates to true if the given executable name can be found in the system PATH, or if an absolute/relative path points directly to an executable file.
Use-case (from the issue): auto-detect the presence of x86_64-linux-musl-gcc when building GraalVM native images with the native-maven-plugin, and conditionally add --static --libc=musl to the build options only when the compiler wrapper is available on the current machine.
Example pom.xml usage:
executable('x86_64-linux-musl-gcc')
Implementation details:
Tests:
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.